Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: optimize informers transformers #1736

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

dongjiang1989
Copy link
Contributor

@dongjiang1989 dongjiang1989 commented Nov 10, 2023

Ⅰ. Describe what this PR does

The optimization has been inspired by cert-manager/cert-manager#5966

By stripping the rest of the metadata, we can save a bit of memory on clusters with lots of pods.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

V. Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in make test

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (93f2bc2) 67.52% compared to head (58125df) 67.53%.

Files Patch % Lines
pkg/util/transformer/metadata_transformer.go 45.83% 11 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1736   +/-   ##
=======================================
  Coverage   67.52%   67.53%           
=======================================
  Files         413      414    +1     
  Lines       46072    46096   +24     
=======================================
+ Hits        31111    31131   +20     
- Misses      12705    12708    +3     
- Partials     2256     2257    +1     
Flag Coverage Δ
unittests 67.53% <45.83%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dongjiang1989
Copy link
Contributor Author

Maybe golangci-lint timeout?

Signed-off-by: dongjiang1989 <[email protected]>
@@ -45,5 +45,9 @@ func SetupTransformers(informerFactory informers.SharedInformerFactory, koordInf
if err := informer.Informer().SetTransform(transformFn); err != nil {
klog.Fatalf("Failed to SetTransform in informer, resource: %v, err: %v", resource, err)
}
// clean up partial metadata
if err := informer.Informer().SetTransform(PartialMetadataRemoveTransform); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If set PartialMetadataRemoveTransform as transformer here, it will replace the exist transformer. It's not the expected.
If you want to do this, you should wrap the transformFn with PartialMetadataRemoveTransform.

@eahydra
Copy link
Member

eahydra commented Nov 13, 2023

@dongjiang1989 Hi, the transformers are now only used by the scheduler. If you want to apply the same functionality in koord-manager and koordlet, you should update more codes.

@dongjiang1989
Copy link
Contributor Author

@dongjiang1989 Hi, the transformers are now only used by the scheduler. If you want to apply the same functionality in koord-manager and koordlet, you should update more codes.

Thanks for your review. Code update immediately!

@koordinator-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign fillzpp after the PR has been reviewed.
You can assign the PR to them by writing /assign @fillzpp in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ZiMengSheng
Copy link
Contributor

/milestone 1.5

@koordinator-bot
Copy link

@ZiMengSheng: The provided milestone is not valid for this repository. Milestones in this repository: [someday, v1.4, v1.5]

Use /milestone clear to clear the milestone.

In response to this:

/milestone 1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ZiMengSheng
Copy link
Contributor

/milestone v1.5

@koordinator-bot koordinator-bot bot added this to the v1.5 milestone Jan 2, 2024
@dongjiang1989
Copy link
Contributor Author

@eahydra Please re-check

@saintube saintube changed the title optimize informers transformers util: optimize informers transformers Jun 3, 2024
@saintube saintube modified the milestones: v1.5, v1.6 Jun 3, 2024
@songtao98
Copy link
Contributor

/assign @ZiMengSheng

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants